Skip to content

[Woo POS] Modularization: refactor POS entry point initializer to minimize public dependencies #16175

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 7 commits into from
Sep 30, 2025
Merged

[Woo POS] Modularization: refactor POS entry point initializer to minimize public dependencies #16175

merged 7 commits into from
Sep 30, 2025

Conversation

@jaclync
Copy link
Contributor

@jaclync jaclync commented Sep 26, 2025 •
edited
Loading

Part of WOOMOB-935

Just one review is required.

This PR should be reviewed after #16173

Overview

This PR refactors the PointOfSaleEntryPointView initializer to minimize the number of POS dependencies that need to be marked as public in preparation for moving POS functionality to a separate module. The goal is to reduce the public API surface by consolidating dependency injection at the entry point level.

Changes Made

  • Refactored PointOfSaleEntryPointView initializer to accept fewer, more consolidated dependencies
  • Moved dependency resolution logic to reduce the number of classes that need public access modifiers

Steps to reproduce

A smoke test in POS would be helpful for regression testing.

Testing Information

  • I tested POS cash and card payment on iPad A16 iOS 26 simulator.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@jaclync
Refactor PointOfSaleEntryPointView initializer to minimize the POS …
f20b499 
…classes that need to be public.
@jaclync
Merge branch 'trunk' into feat/WOOMOB-935-refactor-pos-entry-point
930a88b
@jaclync
Mark necessary code as public in preparation for the move to POS module.
827f23e
@jaclync
Merge branch 'backlog/WOOMOB-869-remove-pos-tab-i2-feature-flag-with-…
e6c65df 
…refactoring' into feat/WOOMOB-935-refactor-pos-entry-point

# Conflicts:
#	WooCommerce/Classes/POS/Presentation/PointOfSaleEntryPointView.swift
#	WooCommerce/Classes/POS/Utils/PreviewHelpers.swift
@dangermattic
Copy link
Collaborator

dangermattic commented Sep 26, 2025 •
edited
Loading

2 Warnings
⚠️ View files have been modified, but no screenshot or video is included in the pull request. Consider adding some for clarity.
⚠️ This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.

Generated by 🚫 Danger

@jaclync jaclync changed the base branch from trunk to backlog/WOOMOB-869-remove-pos-tab-i2-feature-flag-with-refactoring September 26, 2025 00:55
@jaclync jaclync added type: task An internally driven task. feature: POS labels Sep 26, 2025
jaclync
jaclync commented Sep 26, 2025
View reviewed changes
Modules/Sources/Yosemite/PointOfSale/Coupons/PointOfSaleCouponFetchStrategyFactory.swift
import struct NetworkingCore.JetpackSite

public struct PointOfSaleCouponFetchStrategyFactory {
public protocol PointOfSaleCouponFetchStrategyFactoryProtocol {
Copy link
Contributor Author

@jaclync jaclync Sep 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: a protocol was created so that StorageManagerType, a dependency of PointOfSaleCouponFetchStrategyFactory, does not need to be known in the future POS module.

@jaclync jaclync changed the title Refactor POS entry point initializer to minimize public dependencies [Woo POS] Modularization: refactor POS entry point initializer to minimize public dependencies Sep 26, 2025
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Sep 26, 2025 •
edited
Loading

App Icon📲 You can test the changes from this Pull Request in WooCommerce iOS Prototype by scanning the QR code below to install the corresponding build.

App NameWooCommerce iOS Prototype
Build Numberpr16175-a894857
Version23.3
Bundle IDcom.automattic.alpha.woocommerce
Commita894857
Installation URL4ofeh95uep13g
 Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@jaclync jaclync added this to the 23.4 milestone Sep 26, 2025
Base automatically changed from backlog/WOOMOB-869-remove-pos-tab-i2-feature-flag-with-refactoring to trunk September 30, 2025 06:24
@jaclync
Merge branch 'trunk' into feat/WOOMOB-935-refactor-pos-entry-point
a894857 
# Conflicts:
#	WooCommerce/Classes/POS/Models/POSIneligibleReason.swift
#	WooCommerce/Classes/POS/Presentation/Settings/PointOfSaleSettingsView.swift
#	WooCommerce/Classes/POS/TabBar/POSTabCoordinator.swift
@jaclync
Copy link
Contributor Author

jaclync commented Sep 30, 2025 •
edited
Loading

@staskus While resolving the merge conflicts from trunk where GRDBManagerProtocol is now a dependency of POS settings, the future POS module will have to know about GRDBManagerProtocol which is currently in the Storage module along with Core Data code. I don't think GRDB has any dependencies on Core Data or its Storage protocol/implementation, we could move GRDB code somewhere else. Some options I can think of:

  1. Move Storage/GRDB from Storage to a new module like GRDBStorage, and the future POS module depends on it.
  2. Move Storage/GRDB from Storage to the future POS module.
  • Main drawback: no separation of concerns in the POS module, and would be difficult to reuse if the main app adopts GRDB in the future.
  1. Keep Storage/GRDB in Storage, and the future POS module depends on Storage.
  • Main drawback: POS module can access Core Data, manual code reviews are required to prevent misusing Core Data in POS.

WDYT? I personally would go with option 1 given the drawbacks of the other two. Also looping in @joshheald, as I vaguely recall you mentioned separating GRDB into a separate module in a previous PR.

@jaclync jaclync merged commit b5dd3c7 into trunk Sep 30, 2025
13 checks passed
@jaclync jaclync deleted the feat/WOOMOB-935-refactor-pos-entry-point branch September 30, 2025 07:10
@staskus
Copy link
Contributor

staskus commented Sep 30, 2025

@jaclync thanks for those options!

PointOfSale module still depends on Yosemite, which depends on Storage, so fundamentally, I don't think that it's a huge problem that PointOfSale would depend on Storage directly as well.

However, regardless of PointOfSale module, for GRDB it would make sense to have it in a separate module, just as @joshheald pitched to me a few days ago.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@staskus staskus staskus approved these changes

@joshheald joshheald Awaiting requested review from joshheald

Assignees

No one assigned

Labels

feature: POS type: task An internally driven task.

Projects

None yet

Milestone

23.4

Development

Successfully merging this pull request may close these issues.

5 participants

@jaclync @dangermattic @wpmobilebot @staskus